Fix critical bugs in FakeAircraftClass: division by zero, null derefs, and unreachable code #9

Copilot · 2025-12-09T15:00:17Z

Addresses five critical bugs and improves code maintainability in aircraft mission logic.

Critical Bugs Fixed

Division by zero in velocity calculations (lines 629, 728)

const double dist = velocity->Length();
if (dist < AircraftConstants::MinVelocityLength)  // Guard against div/0
    return;
const double scale = aircraftSpeed / dist;

Unreachable code in facing calculation (lines 121-132)

Missing else block caused unconditional overwrite of fac.Raw = 0
Facing angle always computed even when coordinates match

Operator precedence in strafe calculation (line 446)

// Before: Range + 1024 / Speed  (wrong order of operations)
// After:  (Range + 1024) / Speed
return (this->GetWeapon(0)->WeaponType->Range + 1024) / this->Type->Speed;

Null pointer dereferences

_GetWeapon(): pExt not validated before dereference
_FireAt(): pTarget and pTypeExt not validated
_GreatestThreat(): weapon struct pointers not validated

Code Quality Improvements

Named constants: Replaced magic numbers (512, 16, 45, 768, 1024, 0.001) with AircraftConstants namespace
Performance: Cached SelectWeapon() results eliminating 20+ redundant calls in mission loops
Readability: Consistent validation patterns across weapon access paths

Files Changed

src/Ext/Aircraft/Body.h - Constants namespace
src/Ext/Aircraft/Body.cpp - Bug fixes and optimizations
src/Ext/BuildingType/Hooks.DockPoseDir.cpp - Magic number replacement

Original prompt

Overview

This PR addresses several bugs identified in the FakeAircraftClass implementation and related aircraft docking/factory code, along with code quality improvements.

Files to Modify

src/Ext/Aircraft/Body.cpp
src/Ext/Aircraft/Body.h
src/Ext/BuildingType/Hooks.DockPoseDir.cpp

Bug Fixes Required

Bug 1: Unreachable Code in `_Mission_Attack()` (Body.cpp, Lines 121-132)

The code sets fac.Raw = 0 when coordinates match but then continues to overwrite it immediately. The else branch is missing.

Current code:

if (v17.X == v16.X && v17.Y == v16.Y) {
    fac.Raw = 0;
}

auto v18 = std::atan2(double(v17.Y - v16.Y), double(v16.X - v17.X));
auto v19 = Math::DEG90_AS_RAD;
auto v20 = v18 - v19;
auto v21 = Math::BINARY_ANGLE_MAGIC;
fac.Raw = unsigned short(v20 * v21);

Fix: Add else block so the calculation only runs when coordinates don't match:

if (v17.X == v16.X && v17.Y == v16.Y) {
    fac.Raw = 0;
} else {
    auto v18 = std::atan2(double(v17.Y - v16.Y), double(v16.X - v17.X));
    auto v19 = Math::DEG90_AS_RAD;
    auto v20 = v18 - v19;
    auto v21 = Math::BINARY_ANGLE_MAGIC;
    fac.Raw = static_cast<unsigned short>(v20 * v21);
}

Bug 2: Division by Zero Risk in `CalculateVelocity()` (Body.cpp, Line 629)

The calculation aircraftSpeed / dist can cause division by zero or floating-point issues with very small values.

Current code:

const double dist = velocity->Length();
const double scale = aircraftSpeed / dist;

Fix: Add early return for zero/near-zero distance:

const double dist = velocity->Length();
if (dist < 0.001)
    return;
const double scale = aircraftSpeed / dist;

Also apply similar fix around line 727-728:

double finalSpeed = velocity->Length();
if (finalSpeed < 0.001)
    return;
double speedScale = maxBulletSpeed / finalSpeed;

Bug 3: Operator Precedence Error in `_Mission_Attack()` Strafe (Body.cpp, Line 446)

The return value calculation has incorrect operator precedence.

Current code:

return this->GetWeapon(0)->WeaponType->Range + 1024 / this->Type->Speed;

Fix: Add parentheses for correct calculation:

return (this->GetWeapon(0)->WeaponType->Range + 1024) / this->Type->Speed;

Bug 4: Potential Null Pointer in `_GetWeapon()` (Body.cpp, Lines 833-841)

The pExt pointer is not checked for null before dereferencing.

Current code:

WeaponStruct* FakeAircraftClass::_GetWeapon(int weaponIndex)
{
    auto const pExt = AircraftExtContainer::Instance.Find(this);

    if (pExt->CurrentAircraftWeaponIndex >= 0)
        return this->TechnoClass::GetWeapon(pExt->CurrentAircraftWeaponIndex);
    else
        return this->TechnoClass::GetWeapon(this->SelectWeapon(this->Target));
}

Fix: Add null check:

WeaponStruct* FakeAircraftClass::_GetWeapon(int weaponIndex)
{
    auto const pExt = AircraftExtContainer::Instance.Find(this);

    if (pExt && pExt->CurrentAircraftWeaponIndex >= 0)
        return this->TechnoClass::GetWeapon(pExt->CurrentAircraftWeaponIndex);
    
    return this->TechnoClass::GetWeapon(this->SelectWeapon(this->Target));
}

Bug 5: Missing Null Check in `_FireAt()` (Body.cpp, Line 760)

The function should validate that pTarget is not null before proceeding.

Fix: Add null check at the start of _FireAt():

BulletClass* FakeAircraftClass::_FireAt(AbstractClass* pTarget, int nWeaponIdx) {
    if (!pTarget)
        return nullptr;

    auto const pTypeExt = TechnoTypeExtContainer::Instance.Find(this->Type);
    if (!pTypeExt)
        return this->TechnoClass::Fire(pTarget, nWeaponIdx);
    // ... rest of implementation
}

Code Quality Improvements

1. Add Named Constants (Body.cpp or Body.h)

Add a namespace with named constants to replace magic numbers:

namespace AircraftConstants {
    constexpr int MinDistanceForFacing = 512;
    constexpr int MinDistanceToTarget = 16;
    constexpr int FacingErrorTimeout = 45;
    constexpr int LandingProximity = 768;
    constexpr int StrafeCalculationOffset = 1024;
    constexpr double MinVelocityLength = 0.001;
}

Then replace the magic numbers throughout the code:

512 → AircraftConstants::MinDistanceForFacing
16 → AircraftConstants::MinDistanceToTarget
45 → AircraftConstants::FacingErrorTimeout
768 → AircraftConstants::LandingProximity
1024 → AircraftConstants::StrafeCalculationOffset
0.001 → AircraftConstants::MinVelocityLength

2. Add Null Checks in `_GreatestThreat()` (Body.cpp, Lines 503-515)

The weapon pointers should be validated:

Current code:

if (WeaponTypeClass* const pPrimaryWeapon = this->GetWeapon(0)->WeaponType)
    threatType |= pPrimaryWeapon->AllowedThreats();

if (WeaponTypeClass* const pSecondaryWeapon = this->GetWeapon(1)->WeaponType)
    threatType |= pSecondaryWeapon->AllowedThreats();

Fix:

if (auto pWeaponStruct = this->GetWeapon(0))
{
 ...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created as a result of the following prompt from Copilot chat.*
> # Overview
> This PR addresses several bugs identified in the `FakeAircraftClass` implementation and related aircraft docking/factory code, along with code quality improvements.
> 
> ## Files to Modify
> - `src/Ext/Aircraft/Body.cpp`
> - `src/Ext/Aircraft/Body.h`
> - `src/Ext/BuildingType/Hooks.DockPoseDir.cpp`
> 
> ---
> 
> ## Bug Fixes Required
> 
> ### Bug 1: Unreachable Code in `_Mission_Attack()` (Body.cpp, Lines 121-132)
> The code sets `fac.Raw = 0` when coordinates match but then continues to overwrite it immediately. The `else` branch is missing.
> 
> **Current code:**
> ```cpp
> if (v17.X == v16.X && v17.Y == v16.Y) {
>     fac.Raw = 0;
> }
> 
> auto v18 = std::atan2(double(v17.Y - v16.Y), double(v16.X - v17.X));
> auto v19 = Math::DEG90_AS_RAD;
> auto v20 = v18 - v19;
> auto v21 = Math::BINARY_ANGLE_MAGIC;
> fac.Raw = unsigned short(v20 * v21);
> ```
> 
> **Fix:** Add `else` block so the calculation only runs when coordinates don't match:
> ```cpp
> if (v17.X == v16.X && v17.Y == v16.Y) {
>     fac.Raw = 0;
> } else {
>     auto v18 = std::atan2(double(v17.Y - v16.Y), double(v16.X - v17.X));
>     auto v19 = Math::DEG90_AS_RAD;
>     auto v20 = v18 - v19;
>     auto v21 = Math::BINARY_ANGLE_MAGIC;
>     fac.Raw = static_cast<unsigned short>(v20 * v21);
> }
> ```
> 
> ### Bug 2: Division by Zero Risk in `CalculateVelocity()` (Body.cpp, Line 629)
> The calculation `aircraftSpeed / dist` can cause division by zero or floating-point issues with very small values.
> 
> **Current code:**
> ```cpp
> const double dist = velocity->Length();
> const double scale = aircraftSpeed / dist;
> ```
> 
> **Fix:** Add early return for zero/near-zero distance:
> ```cpp
> const double dist = velocity->Length();
> if (dist < 0.001)
>     return;
> const double scale = aircraftSpeed / dist;
> ```
> 
> Also apply similar fix around line 727-728:
> ```cpp
> double finalSpeed = velocity->Length();
> if (finalSpeed < 0.001)
>     return;
> double speedScale = maxBulletSpeed / finalSpeed;
> ```
> 
> ### Bug 3: Operator Precedence Error in `_Mission_Attack()` Strafe (Body.cpp, Line 446)
> The return value calculation has incorrect operator precedence.
> 
> **Current code:**
> ```cpp
> return this->GetWeapon(0)->WeaponType->Range + 1024 / this->Type->Speed;
> ```
> 
> **Fix:** Add parentheses for correct calculation:
> ```cpp
> return (this->GetWeapon(0)->WeaponType->Range + 1024) / this->Type->Speed;
> ```
> 
> ### Bug 4: Potential Null Pointer in `_GetWeapon()` (Body.cpp, Lines 833-841)
> The `pExt` pointer is not checked for null before dereferencing.
> 
> **Current code:**
> ```cpp
> WeaponStruct* FakeAircraftClass::_GetWeapon(int weaponIndex)
> {
>     auto const pExt = AircraftExtContainer::Instance.Find(this);
> 
>     if (pExt->CurrentAircraftWeaponIndex >= 0)
>         return this->TechnoClass::GetWeapon(pExt->CurrentAircraftWeaponIndex);
>     else
>         return this->TechnoClass::GetWeapon(this->SelectWeapon(this->Target));
> }
> ```
> 
> **Fix:** Add null check:
> ```cpp
> WeaponStruct* FakeAircraftClass::_GetWeapon(int weaponIndex)
> {
>     auto const pExt = AircraftExtContainer::Instance.Find(this);
> 
>     if (pExt && pExt->CurrentAircraftWeaponIndex >= 0)
>         return this->TechnoClass::GetWeapon(pExt->CurrentAircraftWeaponIndex);
>     
>     return this->TechnoClass::GetWeapon(this->SelectWeapon(this->Target));
> }
> ```
> 
> ### Bug 5: Missing Null Check in `_FireAt()` (Body.cpp, Line 760)
> The function should validate that `pTarget` is not null before proceeding.
> 
> **Fix:** Add null check at the start of `_FireAt()`:
> ```cpp
> BulletClass* FakeAircraftClass::_FireAt(AbstractClass* pTarget, int nWeaponIdx) {
>     if (!pTarget)
>         return nullptr;
> 
>     auto const pTypeExt = TechnoTypeExtContainer::Instance.Find(this->Type);
>     if (!pTypeExt)
>         return this->TechnoClass::Fire(pTarget, nWeaponIdx);
>     // ... rest of implementation
> }
> ```
> 
> ---
> 
> ## Code Quality Improvements
> 
> ### 1. Add Named Constants (Body.cpp or Body.h)
> Add a namespace with named constants to replace magic numbers:
> 
> ```cpp
> namespace AircraftConstants {
>     constexpr int MinDistanceForFacing = 512;
>     constexpr int MinDistanceToTarget = 16;
>     constexpr int FacingErrorTimeout = 45;
>     constexpr int LandingProximity = 768;
>     constexpr int StrafeCalculationOffset = 1024;
>     constexpr double MinVelocityLength = 0.001;
> }
> ```
> 
> Then replace the magic numbers throughout the code:
> - `512` → `AircraftConstants::MinDistanceForFacing`
> - `16` → `AircraftConstants::MinDistanceToTarget`
> - `45` → `AircraftConstants::FacingErrorTimeout`
> - `768` → `AircraftConstants::LandingProximity`
> - `1024` → `AircraftConstants::StrafeCalculationOffset`
> - `0.001` → `AircraftConstants::MinVelocityLength`
> 
> ### 2. Add Null Checks in `_GreatestThreat()` (Body.cpp, Lines 503-515)
> The weapon pointers should be validated:
> 
> **Current code:**
> ```cpp
> if (WeaponTypeClass* const pPrimaryWeapon = this->GetWeapon(0)->WeaponType)
>     threatType |= pPrimaryWeapon->AllowedThreats();
> 
> if (WeaponTypeClass* const pSecondaryWeapon = this->GetWeapon(1)->WeaponType)
>     threatType |= pSecondaryWeapon->AllowedThreats();
> ```
> 
> **Fix:**
> ```cpp
> if (auto pWeaponStruct = this->GetWeapon(0))
> {
>     if (WeaponTypeClass* const pPrimaryWeapon = pWeaponStruct->WeaponType)
>         threatType |= pPrimaryWeapon->AllowedThreats();
> }
> 
> if (auto pWeaponStruct = this->GetWeapon(1))
> {
>     if (WeaponTypeClass* const pSecondaryWeapon = pWeaponStruct->WeaponType)
>         threatType |= pSecondaryWeapon->AllowedThreats();
> }
> ```
> 
> ### 3. Cache `SelectWeapon()` Result in `_Mission_Attack()` (Body.cpp)
> The `SelectWeapon()` is called multiple times in tight loops. Cache the result:
> 
> In the `FireAtTarget` case (around line 172):
> ```cpp
> case AirAttackStatus::FireAtTarget:
> {
>     if (!this->Target || !this->Ammo)
>     {
>         this->MissionStatus = (int)AirAttackStatus::ReturnToBase;
>         return 1;
>     }
> 
>     const int selectedWeapon = this->SelectWeapon(this->Target);  // Cache this
>     
>     // ... use selectedWeapon instead of calling SelectWeapon() repeatedly
> ```
> 
> Apply similar caching in other cases where `SelectWeapon()` is called multiple times.
> 
> ---
> 
> ## Summary of Changes
> 1. Fix unreachable code in facing calculation
> 2. Add division by zero protection in velocity calculations
> 3. Fix operator precedence in strafe return calculation
> 4. Add null pointer checks throughout
> 5. Add named constants for magic numbers
> 6. Cache repeated function call results for performance
> 

<!-- START COPILOT CODING AGENT TIPS -->
---

✨ Let Copilot coding agent [set things up for you](https://github.com/Otamaa/Phobos-Minimal/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.

- Bug 1: Fix unreachable code in _Mission_Attack() by adding else block - Bug 2: Add division by zero protection in CalculateVelocity() at two locations - Bug 3: Fix operator precedence error in strafe return calculation - Bug 4: Add null pointer check in _GetWeapon() - Bug 5: Add null check in _FireAt() for pTarget and pTypeExt - Add named constants namespace to replace magic numbers (512, 16, 45, 768, 1024, 0.001) - Add null checks in _GreatestThreat() for weapon pointers - Cache SelectWeapon() results in _Mission_Attack() to avoid repeated calls Co-authored-by: deathreaperz <37824218+deathreaperz@users.noreply.github.com>

Initial plan

7fca05a

Copilot AI assigned Copilot and deathreaperz Dec 9, 2025

Copilot started work on behalf of deathreaperz December 9, 2025 15:00 View session

Copilot AI changed the title ~~[WIP] Fix bugs in FakeAircraftClass implementation and improve quality~~ Fix critical bugs in FakeAircraftClass: division by zero, null derefs, and unreachable code Dec 9, 2025

Copilot AI requested a review from deathreaperz December 9, 2025 15:16

Copilot finished work on behalf of deathreaperz December 9, 2025 15:16

Copilot AI mentioned this pull request Jan 13, 2026

Fix critical bugs in FakeTeamClass and YRpp: null pointer crashes, movement completion, script execution, AI stalls, and math operator errors #14

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix critical bugs in FakeAircraftClass: division by zero, null derefs, and unreachable code #9

Fix critical bugs in FakeAircraftClass: division by zero, null derefs, and unreachable code #9

Uh oh!

Copilot AI commented Dec 9, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix critical bugs in FakeAircraftClass: division by zero, null derefs, and unreachable code #9

Are you sure you want to change the base?

Fix critical bugs in FakeAircraftClass: division by zero, null derefs, and unreachable code #9

Uh oh!

Conversation

Copilot AI commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Critical Bugs Fixed

Code Quality Improvements

Files Changed

Overview

Files to Modify

Bug Fixes Required

Bug 1: Unreachable Code in _Mission_Attack() (Body.cpp, Lines 121-132)

Bug 2: Division by Zero Risk in CalculateVelocity() (Body.cpp, Line 629)

Bug 3: Operator Precedence Error in _Mission_Attack() Strafe (Body.cpp, Line 446)

Bug 4: Potential Null Pointer in _GetWeapon() (Body.cpp, Lines 833-841)

Bug 5: Missing Null Check in _FireAt() (Body.cpp, Line 760)

Code Quality Improvements

1. Add Named Constants (Body.cpp or Body.h)

2. Add Null Checks in _GreatestThreat() (Body.cpp, Lines 503-515)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Copilot AI commented Dec 9, 2025 •

edited

Loading

Bug 1: Unreachable Code in `_Mission_Attack()` (Body.cpp, Lines 121-132)

Bug 2: Division by Zero Risk in `CalculateVelocity()` (Body.cpp, Line 629)

Bug 3: Operator Precedence Error in `_Mission_Attack()` Strafe (Body.cpp, Line 446)

Bug 4: Potential Null Pointer in `_GetWeapon()` (Body.cpp, Lines 833-841)

Bug 5: Missing Null Check in `_FireAt()` (Body.cpp, Line 760)

2. Add Null Checks in `_GreatestThreat()` (Body.cpp, Lines 503-515)